Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move the Log, Power functions to datafusion-functions #9983

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

tinfoil-knight
Copy link
Contributor

Which issue does this PR close?

Closes #9875.

Rationale for this change

As part of #9285, log & power functions should be migrated to the datafusion-functions crate.

What changes are included in this PR?

  • Migrates Log & Power from built-in functions & changes to scalar UDFs.
  • The simplification rules for log & power were inter-dependent & therefore were migrated together along with tests.

Are these changes tested?

Tested by existing tests which were migrated (and slightly modified).

Are there any user-facing changes?

No.

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate labels Apr 7, 2024
Expr::ScalarFunction(ScalarFunction {
func_def: ScalarFunctionDefinition::UDF(fun),
args,
}) if base == &args[0] && fun.name() == "power" => {
Copy link
Contributor Author

@tinfoil-knight tinfoil-knight Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Need help]
I wasn't able to figure out how to match the exact PowerFunc type here. Same for a rule in power.rs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can do something like

let is_power = fun.as_any().downcast_ref::<PowerUDF>().is_some();

If that doesn't work I can try and look more into it on Monday

Copy link
Contributor Author

@tinfoil-knight tinfoil-knight Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked with a slight modification. Thank you!

fun.as_ref().inner().as_any().downcast_ref::<PowerFunc>().is_some()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look over the normal things that catch me (monotonicity, aliases, tests) and nothing stood out - LGTM

@tinfoil-knight
Copy link
Contributor Author

I noticed a few things while reviewing the code for merge conflicts and force pushed the commit a couple of times. Sorry for the partial CI re-runs.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @tinfoil-knight and @Omega359 for the review. This is a really nice piece of work and your porting of the simplify logic is quite nice 👌 .

I have a suggestion on how to avoid clone'ing in the simplify logic for your consideration: tinfoil-knight#1 (targets this PR).

However, I think we can also merge this PR as is and then rework the cloning as a follow on.

.downcast_ref::<PowerFunc>()
.is_some() =>
{
Ok(ExprSimplifyResult::Simplified(args[1].clone()))
Copy link
Contributor

@alamb alamb Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really nice to avoid this clone (I am working to reduce the number of times we need to clone Exprs during planning). see tinfoil-knight#1 for a suggestion

@alamb
Copy link
Contributor

alamb commented Apr 8, 2024

I noticed a few things while reviewing the code for merge conflicts and force pushed the commit a couple of times. Sorry for the partial CI re-runs.

No worries -- thanks for your efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move the Log, Power functions to datafusion-functions crate
3 participants